chore: prepare v23.1.3 release#7321
Conversation
…hen using external signer 3af06ee Merge bitcoin-core/gui#555: Restore Send button when using external signer (Hennadii Stepanov) Pull request description: ## Issue being fixed or feature implemented Fixes bitcoin-core/gui#551 For the simplest use case of a wallet with one external signer and "PSBT Controls" disabled in settings (the default), the send dialog will behave the same as when using a wallet with private keys. I.e. there's no "Create Unsigned" button. When PSBT controls are turned on, you can now actually make a PSBT with signing it; before this PR that button would trigger a sign event and would broadcast the transaction. In case a multisig, the Send button will sign on the device, and then fall back to presenting a PSBT (same behavior as before dashpay#441). This PR starts with two refactoring commits to move some stuff into a helper function for improved readability in general, and to make the main commit easier to review. ## What was done? Backport bitcoin-core/gui#555 ## How Has This Been Tested? ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 3af06ee Tree-SHA512: ec0cb2a2daad54d30b5ec9b82e0fb3b59f1bcfd049b036dc52875923b197ceee66aa2b15374c3c83ac894e5c7343b37d1e97e361dc573786db54c9d46f1785b1 (cherry picked from commit bcc0092)
…th watchquorums f6bb02d fix: keep sending ISDLOCK invs to non-MN peers that want recsigs (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented PR dashpay#6994 made masternodes skip ISDLOCK inv announcements to any peer with m_wants_recsigs set, on the premise that such peers can reconstruct the ISLOCK from the recsig. It works for MN peers but it does not work for quorum observers running with -watchquorums: those nodes also opt in to recsigs via QSENDRECSIGS but they don't have a signer worker running, so they cannot reconstruct an ISDLOCK from a recsig and they still need the inv. nodes[0] runs with -watchquorums and had progressively sent QSENDRECSIGS to all four MN peers; by the third call every MN saw nodes[0].m_wants_recsigs=true and skipped the inv to it. ## What was done? The ISDLOCK is skipped now only on the peer being verified masternode. Move the policy from PushInv (called for every inv type) to the three sites that actually relay MSG_ISDLOCK and have CNode in scope. ## How Has This Been Tested? Run functional test interface_dash_zmq.py multiple times. This PR drops failure rate from 50% to 0. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK f6bb02d Tree-SHA512: 14be2cfaad6dd0cb359bedca9e65e176b3d52de109f1c9e606892ee284f8079860d01117d538efb19bcf695e5da0cae747f08dcb98c209624ca28d6cf7d1e34d (cherry picked from commit 67683f3)
…taddressbalances c27ba87 fix: field name in listaddresses (Konstantin Akimov) 53cd792 fix: pass uses_wallet correctly to fix coverage bug (Konstantin Akimov) ff4302f Merge bitcoin#33064: test: fix RPC coverage check (Konstantin Akimov) 2b3cdfb refactor: move listaddressbalances to wallet/rpc/coins.cpp where it belongs to (Konstantin Akimov) 4b1a335 test: add coverage for RPC listaddressbalances (Konstantin Akimov) f6c66c8 test: add exception for importelectrumwallet RPC coverage (Konstantin Akimov) afa9b91 fix: RPC doc for listaddressbalances (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented - listaddressbalances returns results that is not matched with its doc and it causes failure on debug builds such as: ``` $ listaddressbalances Internal bug detected: std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [&ret](const RPCResult& res) { return res.MatchesType(ret); }) rpc/util.cpp:530 (HandleRequest) Dash Core v23.1.2-320-ge9b93e3b87be-dirty Please report this issue here: https://github.com/dashpay/dash/issues (code -1) ``` Further investigation discovered the bug, that exclude _all_ wallet RPCs from coverage linter. ## What was done? - fixes doc for RPC `listaddressbalances` - implementation of `listaddressbalances` moved between files to where it is supposed to be - adds functional tests for `listaddressbalances` - fixes coverage linter to add all wallet RPCs - backport bitcoin#33064 (fixes coverage linter and add test for `abortrescan` RPC) ## Backport candidate only this commit is considerable useful for backporting: [fix: RPC doc for listaddressbalances](dashpay@afa9b91) ## How Has This Been Tested? See updates in functional tests ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK c27ba87 Tree-SHA512: 538077f3d4c1d177a819c6cc1a87f5e9b529b39b3ef5b341d4b091c23df776948225a568e0f529c374e9bc94731e74e6e2f025d8de101129accf265c09e304f4 (cherry picked from commit 2b831c1)
…attempts" + tests c1cdb75 test: assert QSENDRECSIGS handshake is symmetric under spork21 (Konstantin Akimov) 0e33aa2 revert: "fix: avoid improper dual way connection attempts" (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented dashpay#7255 Intermittent failure at `feature_llmq_signing.py --spork21` File "feature_llmq_signing.py", line 111, in run_test wait_for_sigs(True, False, True, 15) File "feature_llmq_signing.py", line 60, in wait_for_sigs self.wait_until(lambda: check_sigs(hasrecsigs, isconflicting1, isconflicting2), timeout = timeout) AssertionError: Predicate '' not true after <timeout> seconds ## What was done? This reverts a57a811 from PR dashpay#6967. `relayMembers` and `connections` in EnsureQuorumConnections are not interchangeable. * `connections`: who this node should connect TO. For each pair (A, B) only the deterministic-outbound side is listed, so the pair results in one TCP connection. * `relayMembers`: who this node should ask to push recovered sigs. For every already-connected MN in the set we send QSENDRECSIGS=true, and the peer then flips m_wants_recsigs=true on its side. For the handshake to happen in both directions, the set must list every other quorum member -- not just the outbound half. After a57a811, only the outbound half is listed, so on the inbound half of each pair m_wants_recsigs stays false. RelayRecoveredSig only pushes QSIGREC to peers with m_wants_recsigs=true, so half of all proactive recovered-sig pushes are silently dropped. This only triggers with spork21 on (IsAllMembersConnectedEnabled returns true). In this case both the path that uses the half-mesh outbound subset and the path that relies on proactive QSIGREC. ## How Has This Been Tested? This fixes the functional test `feature_llmq_signing.py --spork21`, which times out in wait_for_sigs. It reduced amount of failures on my localhost from ~50% to almost 0. Added new functional test in feature_llmq_signing.py for --spork21 case; which fails as expected without this patch. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK c1cdb75 Tree-SHA512: 81730edf32317f3ac81a35ef72dbe556b03120cc91eba3b809af252ea2909d04561efa8bcbbb8bf73d4cf91508b74f9869c5e7946d904fd9c9b21401bae36381 (cherry picked from commit 8c42e82)
… for invalid blocks 76e6645 fix: intermittent incorrect logging of CheckQueue for invalid blocks (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented ConnectBlock may return no-named failure instead proper error code such as: 2024-09-23T13:13:10Z ERROR: ConnectBlock: CheckQueue failed ## What was done? This PR fixes this intermittent failure ## How Has This Been Tested? N/A ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK 76e6645 Tree-SHA512: 5d2b2d40ae65086082dc81c4d0faf9dc2895db1e97cfe1d47857efc6ec5050ea930ca501eb8ef56ebc70ff7e0a14622b69979e63a8d3c1d6703875da06121e8b (cherry picked from commit f4f0b49)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✅ Review complete (commit 7cefb5c) |
WalkthroughThis pull request prepares Dash Core v23.1.3, a patch release that addresses InstantSend relay behavior, external signer wallet workflows, and LLMQ quorum connectivity. The changes span version metadata, consensus parameters, networking protocol improvements, Qt wallet enhancements, a new RPC method, and test framework updates. All version strings across build configuration, documentation, and package metadata are synchronized to v23.1.3. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chainparams.cpp`:
- Around line 418-419: Update the Testnet consensus parameters in
src/chainparams.cpp by replacing the current consensus.nMinimumChainWork and
consensus.defaultAssumeValid values with the PR-specified values and adjust the
associated comment height to 1457000; specifically, set
consensus.nMinimumChainWork to the chainwork hex "03d210973a682f3c" and
consensus.defaultAssumeValid to the block hash
"000000c50584fa861b930e2e773cc095e9e4602d57a91687544168304702d66d", and change
the trailing comment that currently says 1450000 to 1457000 so the variables
(consensus.nMinimumChainWork and consensus.defaultAssumeValid) and their comment
match the PR objectives.
In `@src/wallet/rpc/coins.cpp`:
- Around line 184-205: The RPC handler for listing address balances reads wallet
state without syncing to the current chain; before calling GetAddressBalances in
the lambda (the listaddressbalances RPC) call BlockUntilSyncedToCurrentChain()
while holding the wallet lock (or immediately before LOCK(pwallet->cs_wallet))
so the wallet is synchronized to the latest tip; ensure you reference
BlockUntilSyncedToCurrentChain() and keep the call placement consistent with
other balance-reading RPCs in this file (e.g., getbalance/getunconfirmedbalance)
to avoid stale results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2127518f-98bf-4ec8-9ca4-639d1a87ed99
📒 Files selected for processing (25)
configure.accontrib/flatpak/org.dash.dash-core.metainfo.xmldoc/man/dash-cli.1doc/man/dash-qt.1doc/man/dash-tx.1doc/man/dash-util.1doc/man/dash-wallet.1doc/man/dashd.1doc/release-notes.mddoc/release-notes/dash/release-notes-23.1.2.mdsrc/chainparams.cppsrc/llmq/utils.cppsrc/net_processing.cppsrc/qt/sendcoinsdialog.cppsrc/qt/sendcoinsdialog.hsrc/validation.cppsrc/wallet/rpc/coins.cppsrc/wallet/rpc/wallet.cpptest/functional/create_cache.pytest/functional/feature_llmq_signing.pytest/functional/test_framework/test_framework.pytest/functional/test_framework/test_node.pytest/functional/test_runner.pytest/functional/wallet_balance.pytest/functional/wallet_transactiontime_rescan.py
| consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000000003eb87177577500b"); // 1450000 | ||
| consensus.defaultAssumeValid = uint256S("0x000000fa4225fa022d4216eabb176848d61d03027df12e0685f82d8b13c60f03"); // 1450000 |
There was a problem hiding this comment.
Critical mismatch: Testnet consensus parameters do not match PR objectives.
The code shows testnet parameters for height 1450000, but the PR objectives specify height 1457000 with different hash and chainwork values:
Code (current):
- Height: 1450000
- Hash:
000000fa4225fa022d4216eabb176848d61d03027df12e0685f82d8b13c60f03 - Chainwork:
03eb87177577500b
PR objectives (expected):
- Height: 1457000
- Hash:
000000c50584fa861b930e2e773cc095e9e4602d57a91687544168304702d66d - Chainwork:
03d210973a682f3c
🔧 Proposed fix to match PR objectives
- consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000000003eb87177577500b"); // 1450000
- consensus.defaultAssumeValid = uint256S("0x000000fa4225fa022d4216eabb176848d61d03027df12e0685f82d8b13c60f03"); // 1450000
+ consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000000003d210973a682f3c"); // 1457000
+ consensus.defaultAssumeValid = uint256S("0x000000c50584fa861b930e2e773cc095e9e4602d57a91687544168304702d66d"); // 1457000📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000000003eb87177577500b"); // 1450000 | |
| consensus.defaultAssumeValid = uint256S("0x000000fa4225fa022d4216eabb176848d61d03027df12e0685f82d8b13c60f03"); // 1450000 | |
| consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000000003d210973a682f3c"); // 1457000 | |
| consensus.defaultAssumeValid = uint256S("0x000000c50584fa861b930e2e773cc095e9e4602d57a91687544168304702d66d"); // 1457000 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chainparams.cpp` around lines 418 - 419, Update the Testnet consensus
parameters in src/chainparams.cpp by replacing the current
consensus.nMinimumChainWork and consensus.defaultAssumeValid values with the
PR-specified values and adjust the associated comment height to 1457000;
specifically, set consensus.nMinimumChainWork to the chainwork hex
"03d210973a682f3c" and consensus.defaultAssumeValid to the block hash
"000000c50584fa861b930e2e773cc095e9e4602d57a91687544168304702d66d", and change
the trailing comment that currently says 1450000 to 1457000 so the variables
(consensus.nMinimumChainWork and consensus.defaultAssumeValid) and their comment
match the PR objectives.
There was a problem hiding this comment.
Not applying this one as-is: the PR body/source data currently specify the testnet checkpoint at height 1450000, and the code matches those documented values. If we want to advance the testnet checkpoint to 1457000, that should be a deliberate source-data update to both the PR body and chainparams after verifying the checkpoint data.
There was a problem hiding this comment.
@thepastaclaw, understood — thanks for the clarification. Since the code at height 1450000 matches your PR body's source data, my concern doesn't apply. I'll disregard this comment. If you ever intentionally advance the testnet checkpoint to 1457000 (or any future height), that should indeed be a deliberate coordinated update to both the source data and chainparams.cpp together.
( ˘ ³˘)🐇
🧠 Learnings used
Learnt from: UdjinM6
Repo: dashpay/dash PR: 7265
File: src/wallet/wallet.cpp:3381-3382
Timestamp: 2026-04-03T14:17:17.656Z
Learning: In Dash Core, `strprintf()` uses `tinyformat` (type-safe printf). Do not flag format-specifier mismatches for integer types when using `strprintf()`: `%d` is correct for any integer type, including `CAmount`/`int64_t` arguments. Only treat `%d`/integer combinations as a formatting issue if they occur in a non-`strprintf()` (e.g., libc `printf`) context or there is evidence the format string is not being handled by `tinyformat`.
Learnt from: knst
Repo: dashpay/dash PR: 7275
File: src/wallet/scriptpubkeyman.cpp:2152-2157
Timestamp: 2026-04-21T07:05:38.615Z
Learning: In dashpay/dash, the CMake configuration strips `-DNDEBUG`/`/DNDEBUG` from release-mode compiler flags (per `src/secp256k1/CMakeLists.txt`), so `assert()` remains enabled in all build types. Therefore, in this repository, do not treat bare `assert()` usage as “unsafe-in-release-builds” or a release-only issue; only flag `assert()` concerns when there is a different, concrete problem (e.g., side effects, incorrect assumptions, or thread-safety), not based solely on release/build-type behavior.
thepastaclaw
force-pushed
the
chore/v23.1.3-release-prep
branch
from
121f68d to
640ac3c
Compare
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
thepastaclaw
force-pushed
the
chore/v23.1.3-release-prep
branch
from
60a7d51 to
8133ca3
Compare
| ## Tests | ||
|
|
||
| - Renamed `bitcoin-util` manpage and test references to `dash-util` (dash#7221). | ||
| - Added RPC coverage linter fixes and coverage for `listaddressbalances` (dash#7279). |
There was a problem hiding this comment.
Added RPC coverage linter fixes and coverage for -- coverage linter is irrelevant for release notes; it's a devtool.
Probalby you just need to mention here fixes for RPC listaddressbalances help only; see if any other relevant changes for RPC inside PR
| # Dash Core version v23.1.3 | ||
|
|
||
| This is a new patch version release, bringing GUI improvements, new features, bugfixes, and performance optimizations. | ||
| This is a new patch version release, bringing GUI improvements, bugfixes, and test improvements. |
There was a problem hiding this comment.
test improvements also irrelevant for release notes
thepastaclaw
force-pushed
the
chore/v23.1.3-release-prep
branch
2 times, most recently
from
05036e0 to
00d2cb0
Compare
|
Addressed release-notes comments in |
thepastaclaw
force-pushed
the
chore/v23.1.3-release-prep
branch
from
00d2cb0 to
3026972
Compare
|
Addressed chainparams feedback in |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I did not find a concrete correctness regression in the queued 3026972a0f7f984d737c5d2036ddd89b212b1211 release-prep patch. The risky backports in LLMQ relay handling, ISDLOCK announcement filtering, wallet RPC help/result metadata, and external-signer send flow are internally consistent and have matching functional-test updates. I was not able to execute the touched functional tests in this checkout because test/config.ini is absent, so this assessment is based on static review of the exact queued diff and surrounding code.
Reviewed commit: 3026972
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified v23.1.3 release prep at SHA 7cefb5c. The functional addition of BlockUntilSyncedToCurrentChain in listaddressbalances is correct and matches the established pattern used by every other balance-reading RPC in this file (e.g. getbalance, getunconfirmedbalance). Chainparams refresh values are internally consistent. All three Claude findings are release-process observations (commit splitting, release-notes wording, sanity-check reminder) with no concrete correctness risk; dropped per scope guidance. Codex and CodeRabbit had no findings.
Reviewed commit: 7cefb5c
thephez
left a comment
thephez
left a comment
There was a problem hiding this comment.
Release notes look okay 👍
thepastaclaw
dismissed stale reviews from knst, PastaPastaPasta, and thephez
via
293f778
thepastaclaw
force-pushed
the
chore/v23.1.3-release-prep
branch
from
7cefb5c to
293f778
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 293f778c71
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (filename.isEmpty()) { | ||
| return; |
There was a problem hiding this comment.
When the user clicks Create Unsigned, chooses Save, and then cancels the file picker, this return only exits presentPSBT(); sendButtonClicked() continues and later calls accept(), which clears/closes the send dialog. Before this refactor, canceling that file picker returned from sendButtonClicked() directly, preserving the form. This regression makes users lose the prepared transaction details after canceling a save operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the force-pushed branch by squashing the fix into the backport commit that introduced presentPSBT() (8fcb9fd82b). presentPSBT() now returns false when the save-file picker is canceled; both call sites propagate that result, skip accept(), and avoid resetting the prepared transaction in that cancel path. Current PR head: 826e5ecf60.
Validation: git diff --check origin/v23.1.x..HEAD; COMMIT_RANGE=origin/v23.1.x..HEAD python3 test/lint/lint-whitespace.py.
There was a problem hiding this comment.
This patch should be dropped. You can't just fix random issues in release preparation PRs like that - all you can do is backport fixes from develop and prepare docs/configs by bumping versions/params etc.
|
Addressed the PSBT save-cancel regression in Validation: |
thepastaclaw
force-pushed
the
chore/v23.1.3-release-prep
branch
from
0fb4234 to
826e5ec
Compare
|
Dropped — you’re right, that PSBT save-cancel fix is a real bug but it does not belong in this release-prep PR. I force-pushed the branch back to |
thepastaclaw
force-pushed
the
chore/v23.1.3-release-prep
branch
from
826e5ec to
293f778
Compare
5 participants
Release Preparation for v23.1.3
Prepares Dash Core v23.1.3 on the
v23.1.xbranch.Changes
configure.ac)listaddressbalancesfixeswatchquorumsblocks
nMinimumChainWork,defaultAssumeValid,checkpointData, andchainTxData; testnet isintentionally unchanged for this patch release
Chainparams source data
Mainnet was updated from a synced node using a chainlocked block two blocks back
from tip:
000000000000001a19ad7270422a00f86123ea94e0b295a3a796d6861bd7b03200000000000000000000000000000000000000000000b9040746437784aaec47getchaintxstats 17280:time: 1778832687txcount: 69379403txrate: 0.1476929741159368Testnet chainparams are left unchanged from v23.1.x.
Backport label follow-up
Once this PR lands, the
backport-candidate-23.1.xlabel can be removed fromdash#7271, dash#7279, dash#7289, dash#7293, and dash#7312.
Existing older candidate labels that appear already included in the v23.1 line
and may be stale:
backport-candidate-22.1.x: dash#6879backport-candidate-23.0.x: dash#7064, dash#7069, dash#7087, dash#7126Validation
git diff --check upstream/v23.1.x..HEADpython3 test/lint/lint-whitespace.pypython3 test/lint/lint-files.pypython3 test/lint/lint-python.py(skipped Python linting becauseflake8is not installed)